gh-93357: Lay the foundation for further work in asyncio.test_streams: port server cases to IsolatedAsyncioTestCase#93369
Conversation
|
I've made a rewrite of description for both the PR and the issue to explain a target more clearly. |
|
This is a bit difficult to review since there is a lot of code being refactored. But I will give it a try (is there any other asyncio expert who is interested?). |
sobolevn
left a comment
sobolevn
left a comment
There was a problem hiding this comment.
I forgot to click "Submit" :)
| async def test_eof_feed_when_closing_writer(self): | ||
| # See http://bugs.python.org/issue35065 | ||
| messages = [] | ||
| self.loop.set_exception_handler(lambda loop, ctx: messages.append(ctx)) |
There was a problem hiding this comment.
Looks like .set_exception_handler is now missing from this test. Is it fine?
There was a problem hiding this comment.
Yes, I've removed all usages of messages inter-thread communication list:
-
- messages = [] - self.loop.set_exception_handler(lambda loop, ctx: messages.append(ctx))
-
- self.assertEqual(messages, [])
because the server is now async, lives in a main thread, and needs no intermediary to pass exceptions into a test harness.
There was a problem hiding this comment.
By the way, thank you for noticing, I've added the clarification as the second paragraph of the first PR comment.
gvanrossum
left a comment
gvanrossum
left a comment
There was a problem hiding this comment.
Thanks! I've skimmed the code and it looks fine to me. Together with the expert reviews you've already received and addressed I think it's ready to merge.
I've read your full plan and once this lands please go ahead with the next steps!
|
@gvanrossum This change broke ssl bots on main. See https://github.com/python/cpython/actions/runs/3184337370/jobs/5192630200 |
|
@kumaraditya303 Thank you for reporting; I'm investigating now to fix it ASAP. |
|
Okay, give us updates as you go along -- IIRC the custom is to revert if no fix has been seen within 24 hours. |
|
FWIW my guess would be that you can't actually use |
|
@gvanrossum I've created gh-97896 to fix the break. I'm sorry for the delay; initially I wanted to undo the affected lines but then I ended up with wrapping them into another
|
|
There's a script that runs all the tests that depend on openssl, turning warnings into errors. I ran it manually like this: I agree this ought to be part of the standard CI run, this has bitten me multiple times. |
I will open a new issue for it! |
7 participants
This PR removes ad-hoc implementation of async tests by using all relatively new features of standard
IsolatedAsyncIoTestCase.Edit: this PR also removes
messagesvariable used to pass exceptions between a main and a server threads. That's because the server is now async, lives in a main thread, and needs no intermediary to pass exceptions into a test harness.To make changes possible to review, this PR changes a limited set of tests only. Further commits will expand the change further up and down.
For this, a single test class is temporarily split into three parts; two of them (
StreamTestsandStreamTests2) isolate old cases below and above the upgraded ones, and one (NewStreamTests) are the updated ones themselves. The reason is because subclassing from both oldtest_utils.TestCaseand newunittest.IsolatedAsyncioTestCaseleads to conflicts aborting each test with RuntimeError: cannot enter context: <...> is already entered.test.test_asyncio.utils.TestCasewithunittest.IsolatedAsyncioTestCase#93357